Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overwrite Extraction with Original Folder Structure #212

Merged
merged 13 commits into from
Jan 8, 2025

Conversation

mega5800
Copy link
Contributor

@mega5800 mega5800 commented Dec 12, 2024

👍🎉 First off, thanks for taking the time to contribute! 🎉👍

Please fill out the following checklist:

If you need any help at all, feel free to submit the PR and @-mention activescott and I'll be happy to assist!

Hello @activescott.

I took care of this ticket.

I implemented a logic for detecting and deleting any duplicate files after a file extraction.
I added some tests to cover this feature from different points of view, such as regular extraction, extraction with a defined path and an extraction with specific files.

The newly added tests use a new MSI file I added called “AppleMobileDeviceSupport64.msi”.
I know this file contains duplicate files, thus making it a prefect match for testing this new overwrite extraction.

I’d greatly appreciate if you could review my pull request.
Thank you.

fixes #167

Copy link
Owner

@activescott activescott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mega5800 Great work here. I love the tests! I added a comment to one line that I think we should consider. Let me know what you think.

src/LessMsi.Core/Msi/Wixtracts.cs Outdated Show resolved Hide resolved
@mega5800 mega5800 requested a review from activescott December 21, 2024 21:37
Copy link
Owner

@activescott activescott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I requested one change to simplify the ExtractCommand and make it easier to read. I sent you a PR for the change. Once that is done, I will approve. Thanks!

src/LessMsi.Cli/ExtractCommand.cs Outdated Show resolved Hide resolved
src/LessMsi.Core/Msi/ExtractionMode.cs Show resolved Hide resolved
@activescott
Copy link
Owner

also, please update the branch (you can use the github ui to update it and then pull it again locally from your own branch).

@activescott
Copy link
Owner

@mega5800 Let me know if you have concerns or if there is some way I can help you get this merged. I would like the simplification done, but I can do it if it is too much of a burden for you right now. I appreciate your contributions and I do not want to cause you trouble!

@mega5800
Copy link
Contributor Author

Hello @activescott.

I am currently away and will work on your comments next week.

Happy new year!

@mega5800
Copy link
Contributor Author

mega5800 commented Jan 7, 2025

Hello @activescott.

As you requested, I updated my pull request according to the latest master branch in the main LessMSI repo, accepted your code improvement and made sure we pass all the tests.

I’ve also replied to your comment regarding the change in the Core project.Please let me know if you have any additional comments about that topic. Besides that, I think we are ready for a merge.

Thank you.

@activescott activescott merged commit c5944b4 into activescott:master Jan 8, 2025
2 checks passed
@activescott
Copy link
Owner

🎉 This PR is included in version 2.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an overwrite option
2 participants